Possibly over-thinking it, but the this got me wondering if rather than a subclass, this would work nicer as a method that took a processor / callback-handler style argument. The persister would then just need 1 of those while it worked during startup and then it would go away when done, unlike this. You could also then separately test the codec class (current parent) and the processor/callback fully that way rather than e.g creating a test subclass to test the parent bits, and not really directly testing this class at all [it seems so far].

The large and regular persister could still use the same processor and then just toggle a flag to indicate which behaviour to give.

@gemmellr I can do that. although I'm using a thread local now because of the state I needed to store before creating a message.

There's one changed I wanted to make on the constructors for AMQPStandardMessage and AMQPLargeMessage, that I could keep the implementation stateless, and just post data directly on the message. then I wouldn't need a ThreadLocal and I could just work with the message directly.

not sure about a callback. I tried that before I sent the PR and I didn't like it.

@gemmellr I played with extension versus plugin.. it doesn't change much in the code.

We will still have a single ThreadLocal... I will keep it as extension for now

I dont recall any need for a ThreadLocal in the approach my comment wondered about, pretty sure removing that that was what made me think of it. Unclear what would you be storing in it with the state in the processor/callback and the rest being static methods or maybe a singleton. It also seemed like it would fairly significantly change to the code and the [direct] testing, particularly of the current subclass.

The main issue is that I need to know the messageFormat before creating an instance of AMQPStandardMessage or AMQPLargeMessage.

I have relaxed this a bit now and won't require any threadLocals any longer and will just parse it directly onto the mesasge object. (I will be push the commit shortly)

@gemmellr I tried decoding directly into the message but it made things worse.

Historically we've always created the message first, then applied reload. Inverting this for AMQPStandardMessage seemed doable but required a lot of new code. For AMQPLargeMessage it's not feasible at all—the messageID is tied to a file, and adding reload methods just creates more problems than it solves.

I'd keep the current approach with ThreadLocal state. If you want the codec stateless, we'd still need a ThreadLocal or temporary instance to hold the data before message instantiation anyway.

I dont recall any need for a ThreadLocal in the approach my comment wondered about, pretty sure removing that that was what made me think of it. Unclear what would you be storing in it with the state in the processor/callback and the rest being static methods or maybe a singleton. It also seemed like it would fairly significantly change to the code and the [direct] testing, particularly of the current subclass.